Repository navigation
stress: give the marker tile's reading the width the chip was holding - #2172
Merged
Merged
Conversation
The Resting HR and HRV tiles clipped both halves of their row: "53 …" beside "▲ +7 vs …" where the tile should read "53 bpm" and "▲ +7". The two tiles on the same rows without a delta chip, Stress and Calm time, were never affected, which is what places it in the row that holds a value beside a chip. #492 item 5 had already been here. Before it the chip took its natural width, about 90dp of a row with about 140dp to give, and left the reading as "4…", so the row was set to split evenly instead. That starves both sides rather than one: a weighted child with fill = true is held to exactly its share, so the value sits at half however little the chip needs, and half is not enough for either "53 bpm" or "+7 vs base". The chip now carries the signed magnitude and nothing else. The section header directly above it already reads "vs 30-day baseline", so the words were being bought at the reading's expense to say something the reader had just been given. At 12sp with at most three digits the chip measures under 45dp, which leaves the value the rest of the row at full size, so the even split goes and the natural-width path comes back. "at baseline" becomes "±0" for the same reason, and TrendChip shows no glyph for it, having one only for a signed value. Both strings were English literals on Android while the Apple twin passes both through String(localized:), so this also takes two untranslated words out of the UI without needing a new string. Tools/i18n_audit.py never saw them, its count being 234 before and after, which is the known blind spot for literals assigned outside a composable call. No caller passes compactDelta now. Its documentation says so, and says to reach for a shorter chip first.
Re-review. The tile claimed a fact it did not have. rhrDelta is null when today has no reading or there is no 30-day baseline to stand it against, and both fell through to the at-baseline chip, so a tile with nothing in it put the at-baseline chip beside the empty-value dash, and a first-week tile put a reading exactly on a baseline that did not exist yet. Null now draws no chip at all, which both StatTiles already support: the Kotlin one renders the pill inside `if (delta != null)` and the Swift one inside `if let delta`. The at-baseline branch is reached only with a real delta now, so it says "measured, and it is zero" rather than standing in for "nothing to measure". The same flaw was in the Swift twin, so both are done together rather than leaving one side claiming it. iOS keeps its wording, not being the surface that clips; the shortened chip stays Android only. Not caught by the previous pass because the screenshot came from a strap with a month of history behind it, where the delta is always present.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What was wrong
Reported by @bartmuskala with a screenshot that placed it exactly. On the Stress screen the
RESTING HRtile read53 …beside a chip reading▲ +7 vs …, andHRVread31 …beside▼ −29 v…. Both halves of the row cut, neither winning.The two tiles on the same rows that carry no delta chip,
STRESSandCALM TIME, render in full at the same width. That places it in the row that holds a value beside a chip rather than in the tile or the screen.Why
MarkerTileasked forcompactDelta, which splits that row evenly. On a two-up tile there is about 140dp to give, and+7 vs basealone wants about 90dp of it, so neither side fits in its half.The even split was itself the previous repair. Before #492 item 5 the chip took its natural width and left the reading as
4…, so the row was set to share instead. Sharing starves both sides rather than one, because a weighted child withfill = trueis held to exactly its share: the value sits at half however little the chip turns out to need. There was no width at which both strings fit, so the row could only pick which one to cut.The change
The chip carries the signed magnitude and nothing else. The section header directly above these tiles already reads
vs 30-day baseline, sovs basewas being bought at the reading's expense to repeat something the reader had just been given. At 12sp with at most three digits the chip measures under 45dp including its pill padding, which leaves the value the rest of the row at full size. The even split then goes, and the natural-width path comes back.at baselinebecomes±0for the same reason.TrendChipshows no glyph for it, having one only for a signed value.No caller passes
compactDeltaany more. Its documentation now says so, and says to reach for a shorter chip before reaching for it, since the workouts feed had already left it behind by going full width.A second bug, not fixed here
The value is drawn by
AutoSizeValue, whose stated job is to shrink to 0.6x rather than truncate, added for this exact symptom in #319/#332.53 bpmat 0.6x would have fitted the half it was given. It truncated instead, so that safety net is not firing, and it sits under everyStatTilein the app rather than only these two.This PR does not depend on it: the reading now fits at full size with no shrink required. I have not chased the cause, because it wants a device to confirm and I cannot run Compose here. Filed separately as #2171.
Localisation
vs baseandat baselinewere English literals on Android while the Apple twin passes both throughString(localized:). The change takes both out of the UI without needing a new string, since a signed number needs no translation.Tools/i18n_audit.pynever saw them: its count is 234 before and after. That is the known blind spot for literals assigned outside a composable call, so the audit passing was not evidence they were fine.Type of change
How it was tested
compileFullDebugKotlinclean, doc lint clean,i18n_audit.pyunchanged at 234.This is a layout change and wants eyes on a device, which is the honest state of it: there is no unit test that measures a Compose row, and the screenshot that reports it came from a phone I do not have. What to look at on the Stress screen:
RESTING HRandHRVshould read53 bpmand31 msin full, with a short chip beside each, and should stay intact at larger font scales, where the old split failed first.One trade-off worth stating
A screen reader used to hear
+7 vs baseand now hears+7. The tile label and the section header still carry the context, but the chip alone is terser than it was. Restoring it properly means a description string localised across the eightvalues*files rather than a literal, which is more churn than the two words are worth in this PR. Happy to add it if you would rather have it.Related issues
#2145. #2171 is the shrink helper, which is the wider of the two.
Re-review (85cb48d)
The tile claimed a fact it did not have.
rhrDeltais null when today has no reading or there is no 30-day baseline to stand it against:Both of those fell through to the at-baseline chip, so a tile with nothing in it put that chip beside the empty-value dash, and a first-week tile put a reading exactly on a baseline that did not exist yet. Shortening the chip to
±0sharpened the false claim rather than causing it, which is what made it visible on a second read.A null delta now draws no chip at all, which both
StatTiles already support: the Kotlin one renders the pill insideif (delta != null), the Swift one insideif let delta. The at-baseline branch is reached only with a real delta now, so it says "measured, and it is zero" rather than standing in for "nothing to measure".The Swift twin had the identical flaw, so both are corrected together rather than leaving one side claiming it. This widens the PR past the Android layout it started as, deliberately: it is one defect in one tile with two implementations. iOS keeps its wording, not being the surface that clips, so the shortened chip stays Android only.
The first pass missed it because the reported screenshot came from a strap with a month of history behind it, where the delta is always present. The empty and first-week states are the ones nobody photographs.
Checked: parity ledger unmoved,
Strand/**being outside the scanned globs. Kotlin compiles, doc lint clean. The Swift change is app-target, which does not build on this machine, so it rests on the macOS leg of CI. Adding a Swift file also moves this PR onto the larger check roster.